add C# deterministic serialization #13160

fmg-lydonchandra · 2023-06-28T06:42:19Z

@jskeet review please :-) as this is my first contrib to protobuf, any design / performance feedback is very sought after

google-cla · 2023-06-28T06:42:23Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

fmg-lydonchandra · 2023-06-28T06:54:56Z

csharp/src/Google.Protobuf/Collections/MapField.cs

@@ -452,7 +452,17 @@ public void WriteTo(CodedOutputStream output, Codec codec)
            WriteContext.Initialize(output, out WriteContext ctx);
            try
            {
-                WriteTo(ref ctx, codec);
+                if (output.IsSerializationDeterministic())


How do I unit-test this ? Is this for old proto2 format ?

I'm not sure what you mean by that. Is what for proto2 format?

fmg-lydonchandra · 2023-06-28T06:56:00Z

csharp/src/Google.Protobuf.Test/GeneratedMessageTest.cs

@@ -691,6 +691,54 @@ public void OneofSerialization_DefaultValue()
            });
        }

+        [Test]
+        public void MapSerializationDeterministicTrue_ThenBytesIdentical()


Will add more tests for different types of keys, once the general implementation approach is approved.

jskeet · 2023-06-28T07:01:40Z

I'll get to this when I can - it may not be this week though.

jskeet

Various suggestions, but I'm happy with the overall approach.

jskeet · 2023-06-28T09:30:30Z

csharp/src/Google.Protobuf/CodedOutputStream.cs

+        /// maps are sorted on the lexicographical order of the UTF8 encoded keys.</description></item>
+        /// </list>
+        /// </remarks>
+        public void UseDeterministicSerialization() {


This should just be a property:

public bool DeterministicSerialization { get; set; }

I'm not hugely keen on the stream being mutable like this, but it appears to be the design in other languages.

(Naming could be IsDeterministic or just Deterministic... I'm not hugely bothered on that front.)

Refactored to property: Deterministic

jskeet · 2023-06-28T09:32:13Z

csharp/src/Google.Protobuf/Collections/MapField.cs

@@ -452,7 +452,17 @@ public void WriteTo(CodedOutputStream output, Codec codec)
            WriteContext.Initialize(output, out WriteContext ctx);
            try
            {
-                WriteTo(ref ctx, codec);
+                if (output.IsSerializationDeterministic())


I'm not sure what you mean by that. Is what for proto2 format?

jskeet · 2023-06-28T09:32:27Z

csharp/src/Google.Protobuf/CodedOutputStream.cs

+        /// change in the future)
+        /// <list type="bullet">
+        /// <item><description>Sort map entries by keys in lexicographical order or numerical order. Note: For string
+        /// keys, the order is based on comparing the Unicode value of each character in the strings.


Each Unicode character, or each UTF-16 code unit?

Updated to UTF-16 code unit

jskeet · 2023-06-28T09:35:47Z

csharp/src/Google.Protobuf/Collections/MapField.cs

+                }
+                else
+                {
+                    WriteTo(ref ctx, codec, list);


I think it would be better not to duplicate the WriteTo call here. Something like:

IList<KeyValuePair<TKey, TValue>> listToWrite = list; if (output.IsDeterministic) { listToWrite = new List<KeyValuePair<TKey, TValue>>(list); listToWrite.Sort(...); } WriteTo(ref ctx, codec, listToWrite);

refactored to call GetSortedListCopy

jskeet · 2023-06-28T09:36:37Z

csharp/src/Google.Protobuf/Collections/MapField.cs

@@ -469,7 +479,24 @@ public void WriteTo(CodedOutputStream output, Codec codec)
        [SecuritySafeCritical]
        public void WriteTo(ref WriteContext ctx, Codec codec)
        {
-            foreach (var entry in list)
+            if (ctx.state.CodedOutputStream is not null && ctx.state.CodedOutputStream.IsSerializationDeterministic())


This can just be:

if (ctx.state.CodedOutputStream?.IsDeterministic == true)

Refactored to if (ctx.state.CodedOutputStream?.Deterministic ?? false)

jskeet · 2023-06-28T09:37:04Z

csharp/src/Google.Protobuf/Collections/MapField.cs

+                // We can't sort the list in place, as that would invalidate the linked list.
+                // Instead, we create a new list, sort that, and then write it out.
+                var sorted = new List<KeyValuePair<TKey, TValue>>(list);
+                sorted.Sort((pair1, pair2) => Comparer<TKey>.Default.Compare(pair1.Key, pair2.Key));


Given the repetition here, I'd probably extract the logic into a method of GetSortedListCopy or similar. Otherwise if we tweaked how the comparer is set up, it could end up inconsistent.

refactored to GetSortedListCopy

jskeet · 2023-06-28T09:40:53Z

csharp/src/Google.Protobuf/Collections/MapField.cs

+                if (output.IsSerializationDeterministic())
+                {
+                    var sorted = new List<KeyValuePair<TKey, TValue>>(list);
+                    sorted.Sort((pair1, pair2) => Comparer<TKey>.Default.Compare(pair1.Key, pair2.Key));


Using Comparer.Default begs the question as to whether the default string comparison is ordinal or not. I believe it is, but I'm not entirely sure.

I'd at least add a comment here - and possibly also note that map keys are guaranteed to be bools, integers or strings; never bytestrings or messages.

It is NOT ordinal by default, explicitly using StringComparer.Ordinal now

jskeet · 2023-06-29T10:37:29Z

Thanks for the additional commit - I'll review on Monday.

jskeet

Yup, this looks good to me. Will rerun tests.

jskeet · 2023-07-03T13:15:05Z

csharp/src/Google.Protobuf/CodedOutputStream.cs

@@ -160,6 +160,33 @@ public long Position
            }
        }

+        /// <summary>
+        /// Configures serialization to be deterministic.


Perhaps "Configures whether or not serialization is deterministic." (It's a property, but the current text sounds sounds like it's a method.)

jskeet · 2023-07-03T13:15:14Z

csharp/src/Google.Protobuf/CodedOutputStream.cs

+        /// The deterministic serialization guarantees that for a given binary, equal messages (defined by the
+        /// equals methods in protos) will always be serialized to the same bytes. This implies:
+        /// <list type="bullet">
+        /// <item><description>Repeated serialization of a message will return the same bytes</description></item>


Nit: add period

jskeet · 2023-07-03T13:15:27Z

csharp/src/Google.Protobuf/CodedOutputStream.cs

+        /// Configures serialization to be deterministic.
+        /// </summary>
+        /// <remarks>
+        /// The deterministic serialization guarantees that for a given binary, equal messages (defined by the


Nit: "The deterministic serialization" => "Deterministic serialization"

jskeet · 2023-07-03T13:47:37Z

The tests appear to be failing in CI - I'll take a look.

@jskeet

#12881 @jskeet review please :-) as this is my first contrib to protobuf, any design / performance feedback is very sought after Closes #13160 COPYBARA_INTEGRATE_REVIEW=#13160 from fmg-lydonchandra:feature/12881_cs_serialization_deterministic ab7e01b FUTURE_COPYBARA_INTEGRATE_REVIEW=#13160 from fmg-lydonchandra:feature/12881_cs_serialization_deterministic ab7e01b PiperOrigin-RevId: 567635727

@jskeet

#12881 @jskeet review please :-) as this is my first contrib to protobuf, any design / performance feedback is very sought after Closes #13160 COPYBARA_INTEGRATE_REVIEW=#13160 from fmg-lydonchandra:feature/12881_cs_serialization_deterministic ab7e01b FUTURE_COPYBARA_INTEGRATE_REVIEW=#13160 from fmg-lydonchandra:feature/12881_cs_serialization_deterministic ab7e01b PiperOrigin-RevId: 567635727

add basic deterministic serialization

1092b7d

removed unused WriteDeterministicTo

13087fe

fmg-lydonchandra commented Jun 28, 2023

View reviewed changes

fmg-lydonchandra marked this pull request as ready for review June 28, 2023 07:00

fmg-lydonchandra requested a review from a team as a code owner June 28, 2023 07:00

fmg-lydonchandra requested review from jskeet and removed request for a team June 28, 2023 07:00

jskeet reviewed Jun 28, 2023

View reviewed changes

mkruskal-google added c# 🅰️ safe for tests Mark a commit as safe to run presubmits over labels Jun 28, 2023

github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jun 28, 2023

PR Review feedback take protocolbuffers#1

590c61d

Use StringComparer.Ordinal to sort string keys

f2e37d4

jskeet approved these changes Jul 3, 2023

View reviewed changes

jskeet added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jul 3, 2023

github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jul 3, 2023

Updated xml docs as per suggestion

1e2e6e9

jskeet assigned jskeet and deannagarcia Jul 3, 2023

jskeet added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jul 3, 2023

github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jul 3, 2023

add SecuritySafeCritical attribute for Net462 to fix failing tests

ab7e01b

jskeet added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jul 3, 2023

github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jul 3, 2023

jskeet unassigned deannagarcia Sep 19, 2023

jskeet requested a review from mkruskal-google September 20, 2023 07:54

mkruskal-google approved these changes Sep 22, 2023

View reviewed changes

copybara-service bot mentioned this pull request Sep 22, 2023

add C# deterministic serialization (#13160) #14180

Closed

copybara-service bot closed this in 96100bf Sep 26, 2023

jskeet mentioned this pull request Nov 13, 2023

Add c# serializationDeterministic #12881

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add C# deterministic serialization #13160

add C# deterministic serialization #13160

fmg-lydonchandra commented Jun 28, 2023 •

edited

Loading

google-cla bot commented Jun 28, 2023

fmg-lydonchandra Jun 28, 2023

jskeet Jun 28, 2023

fmg-lydonchandra Jun 28, 2023

jskeet commented Jun 28, 2023

jskeet left a comment

jskeet Jun 28, 2023

fmg-lydonchandra Jul 3, 2023

jskeet Jun 28, 2023

jskeet Jun 28, 2023

fmg-lydonchandra Jul 3, 2023

jskeet Jun 28, 2023

fmg-lydonchandra Jul 3, 2023

jskeet Jun 28, 2023

fmg-lydonchandra Jul 3, 2023

jskeet Jun 28, 2023

fmg-lydonchandra Jul 3, 2023

jskeet Jun 28, 2023

fmg-lydonchandra Jul 3, 2023

jskeet commented Jun 29, 2023

jskeet left a comment

jskeet Jul 3, 2023

jskeet Jul 3, 2023

jskeet Jul 3, 2023

jskeet commented Jul 3, 2023

add C# deterministic serialization #13160

add C# deterministic serialization #13160

Conversation

fmg-lydonchandra commented Jun 28, 2023 • edited Loading

google-cla bot commented Jun 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jskeet commented Jun 28, 2023

jskeet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jskeet commented Jun 29, 2023

jskeet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jskeet commented Jul 3, 2023

fmg-lydonchandra commented Jun 28, 2023 •

edited

Loading